-
Notifications
You must be signed in to change notification settings - Fork 3k
Component support #451
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Component support #451
Conversation
No rush, but I have a certain fear that the angular community only wants to support bower, @nateabele can you hint at whether you'll generally support component or not? |
@jasonkuhrt Regarding the general Angular community, it seems so, yes. Personally I'm pretty ambivalent to Bower, and find the setup to be more trouble than it's worth. My hesitancy at this particular PR has more to do with the fact that it rewrites half the readme. Also, squash your commits. |
@nateabele I am already aware I should squash my commits, but assumed discussion would be needed, and it is.
Sorry, but that's not true. I've literally only added 4 new lines of text: https://github.com/littlebitselectronics/ui-router/tree/component-support#get-started You'll see that I tried two versions (and several more uncommitted). Its not easy. My initial attempt was to make component instructions part of your existing 3-step-process but that doesn't work at all because component doesn't share the same steps. The sane path was to add component as its own entry. Please provide feedback regarding how I can improve the readme, I am more than happy to do so. |
[and thus you can set up the dependency by doing require('angular-ui-router') within the angular module declaration.] I understand why it's possible to do this in cases where a module/component/widget has already put itself in the global scope. Is this a standard practice for "component" components? In other words, would component users be familiar with this technique? (I use RequireJS quite a bit and haven't seen that done, even though it's possible.) I guess if you're going to export the module name, one could argue that ui-router might as well detect AMD and return the module name in the same manner. Thanks. |
Sorry, it's tough to assess PRs multiple times a day across multiple projects. All I saw was a big wall of red and green across the
It's easier if you use GitHub's online editor. It lets you flip between code & preview.
Here, I fixed it. Let me know if there any details missing. diff --git a/README.md b/README.md
index bf9d2d1..54528f5 100644
--- a/README.md
+++ b/README.md
@@ -29,11 +29,12 @@ to change. Using it in a project that requires guaranteed stability is not recom
**(1)** Get UI-Router in one of 3 ways:
- clone & [build](#developing) this repository
- [download the release](http://angular-ui.github.io/ui-router/release/angular-ui-router.js) (or [minified](http://angular-ui.github.io/ui-router/release/angular-ui-router.min.js))
- - or via **[Bower](http://bower.io/)**: by running `$ bower install angular-ui-router` from your console
+ - via **[Bower](http://bower.io/)**: by running `$ bower install angular-ui-router` from your console
+ - or via **[Component](https://github.com/component/component)** by running `$ component install angular-ui/ui-router` from your console
**(2)** Include `angular-ui-router.js` (or `angular-ui-router.min.js`) in your `index.html`, after including Angular itself
-**(3)** Add `'ui.router'` to your main module's list of dependencies
+**(3)** Add `'ui.router'` to your main module's list of dependencies (if you're using Component, replace `'ui.router'` with `require('angular-ui-router')`)
When you're done, your setup should look similar to the following:
@@ -46,6 +47,8 @@ When you're done, your setup should look similar to the following:
<script src="js/angular-ui-router.min.js"></script>
<script>
var myApp = angular.module('myApp', ['ui.router']);
+ // For Component users, it should look like this:
+ // var myApp = angular.module('myApp', [require('angular-ui-router')]);
</script>
...
</head> |
@nateabele Thanks, that looks totally acceptable. @stu-salsbury Hey, I'm not sure I accurately follow your line of thought.
We should not conflate component users and component-angular users. Angular has the unique notion of When With all this mind, I'm not sure what a more predictable pattern would be for integrating
The best case with least effort that I can think of is to have every component-angular-x register itself with angular and export its module name. Some others came to the same conclusion: https://gist.github.com/Enome/6194699 I'd be happy to hear others' thoughts on this of course: @ianstormtaylor @Enome @hugojosefson
Sure? Sounds good to me. |
I guess I was just thinking that exporting the module name in order to use it in the module dependencies list looked a little bit like voodoo from an angular perspective. I've been known to use voodoo myself, so not a slight -- I was just wondering whether it was common practice when you're dealing with a component that intends to register itself in the global context (or in the case of angular, as a module) to export the name at which the component can be located (in angular's case as a module name, in window's case as a window property name). |
@stu-salsbury Understandable. FYI this is also another perfectly doable and okay pattern:
|
Of course -- I actually find the voodoo attractive and hoped to hear that it was standard practice :) |
@stu-salsbury Still early days in terms of angular + component so hard to say what is standard. I would vote for this pattern as what else can you usefully return? Its helpful that this pattern still lets you use non-main module modules, i.e. look at the other things you could use from So this is fully supported:
|
@jasonkuhrt Sounds good to me. Ping me when you get the readme & commits cleaned up and I'll get this merged. Also, let me know what I need to do to register a component module. |
@nateabele @stu-salsbury Should we incorporate AMD support or leave that for another issue? |
From my perspective amd support isnt necessary bc it works fine with requirejs shim. |
@stu-salsbury Thanks for chiming in, I have no valuable opinion here. Then lets not push anything at least within this issue. @nateabele thanks, I will tidy things up and ping you. |
@jasonkuhrt The Gruntfile changes? Fine with me, as long as they don't impact anything else. |
@nateabele Hey, yeah for two things:
1 is innocuous but 2 should be double-checked by someone else to ensure that I am no introducing unexpected results in minification/concatenation edge-cases. @stu-salsbury @nateabele Mind reviewing this?: littlebitselectronics@c5a724f#L0R28 I expect it to end up like this:
|
@nateabele I git |
The result should be that the changes are all the same, but all in one commit. You should then have to force-push that commit to your remote branch. |
@nateabele Done, thanks for the tip. |
@nateabele Just a note that until a release build is done the component support won't work since the |
Yup, I'm aware. We'll probably be doing a release sometime this week. Also, what do I need to do to get this thing listed in Component's registry? |
Great
Add your project to this wiki: https://github.com/component/component/wiki/Components#angularjs. I'm assuming you'd add it under the angularjs section. |
This adds support for component i.e.:
I've updated:
grunt
concat
's "banner" to include code to supportcommonjs
.grunt
prerelease
to ensurecomponent.version
is synced up.readme.md
to includecomponent
instructions.Let me know how to improve this or if its good to go. Note that I have not cut an actual release which appears to be something inappropriate to do myself.
Thanks.